wallet2: use a gamma distribution to pick fake outs #3528

moneromooo-monero · 2018-03-30T21:41:44Z

as per "An Empirical Analysis of Linkability in the Monero
Blockchain", by Miller et al.

vtnerd

Has anyone confirmed the findings of this paper? Is the gamma distribution actually as described? I see no reason to suspect intentionally incorrect findings, and the conclusions seem correct, but I also haven't run the same analysis on the blockchain.

vtnerd · 2018-04-03T03:13:05Z

src/wallet/wallet2.cpp

+    for (size_t idx: selected_transfers)
+      if (m_transfers[idx].is_rct())
+        has_rct = true;
+    bool has_rct_distribution = has_rct && get_rct_distribution(rct_start_height, rct_offsets);


This can be const, and probably shouldn't be modified after this point.

vtnerd · 2018-04-03T03:14:28Z

src/wallet/wallet2.cpp

-    THROW_WALLET_EXCEPTION_IF(resp_t.status == CORE_RPC_STATUS_BUSY, error::daemon_busy, "get_output_histogram");
-    THROW_WALLET_EXCEPTION_IF(resp_t.status != CORE_RPC_STATUS_OK, error::get_histogram_error, resp_t.status);
+      if (!m_transfers[idx].is_rct() || !has_rct_distribution)
+        req_t.amounts.push_back(m_transfers[idx].is_rct() ? 0 : m_transfers[idx].amount());


This ternary operation should be pointless now.

Because amount() will return 0 for rct ? Yes, that seems right, but there are other places where the amount is known (when you're sending) and I feel a bit uneasy not having this explicit case here if it inspires other place. But I think technically you're right.

I'm pretty sure that m_amount (which is the same as amount()) for rct outputs is the decoded amount, not zero.

monero/src/wallet/wallet2.cpp

Line 1362 in a9b83f5

td.m_amount = tx_scan_info[o].amount;

vtnerd · 2018-04-03T22:19:37Z

src/wallet/wallet2.cpp

+      uint64_t block_offset = x / DIFFICULTY_TARGET_V2; // this assumes constant target over the whole rct range
+      if (block_offset >= rct_offsets.size() - 1)
+        return std::numeric_limits<uint64_t>::max(); // bad pick
+      block_offset = rct_offsets.size() - 2 - block_offset;


Can this select outputs that are still locked? The rct_offsets looks to be the up to the latest block, and I think the num_outs call is filtering away locked outputs below. I think this is correct behavior but I wanted to start some discussion based on the recommendations from the paper.

Specifically my question is whether the gamma distribution should be over just the unlocked outputs, or over all outputs. It looks like the paper just tracked the spend time of 0 and 1 mixin outputs, and noted that the time distribution was different than Bitcoin. Which is to be expected, since Bitcoin outputs do not have a lock period. So again, I think the authors intended the parameters to be used over all outputs, but I was curious whether anyone else had information on this topic.

This can pick locked outputs indeed, and they're rejected later. This is necessary also because coinbase outs are locked for 60 blocks, so all locked outs aren't necessarily at the end. In any case, I think using one or the other would yield a very similar distribution anyway (just a feeling, no evidence).

moneromooo-monero · 2018-04-04T11:13:29Z

One thing I've confirmed is that the (pre-dupe-rejection) distribution gets us roughly half of pick within the last 1.8 days. Those 1.8 days were estimated from the paper's graph though, but it means the code seems to match the claimed distribution, at least.

naughtyfox · 2018-06-29T10:06:02Z

src/wallet/wallet2.cpp

+    bool has_rct = false;
+    for (size_t idx: selected_transfers)
+      if (m_transfers[idx].is_rct())
+        has_rct = true;


why don't break the cycle after the condition is met? no need to iterate further

No particular reason either way.

stoffu · 2018-07-02T06:52:36Z

src/wallet/wallet2.cpp

+      double min() const { return 0.0; }
+      double max() const { return 1.0; }
+      double operator()() { return crypto::rand<uint64_t>() / (double)std::numeric_limits<uint64_t>::max(); }
+    } engine;


This gives the following compile error:

In file included from /Users/buildbot/monero-builder/monero-static-osx-10_13/build/src/wallet/wallet2.cpp:32: /Library/Developer/CommandLineTools/usr/include/c++/v1/random:3643:51: error: call to non-static member function without an object argument const size_t __logR = __log2<uint64_t, _URNG::max() - _URNG::min() + uint64_t(1)>::value; ~~~~~~~^~~ /Library/Developer/CommandLineTools/usr/include/c++/v1/random:4167:30: note: in instantiation of function template specialization 'std::__1::generate_canonical<double, 53, gamma_engine>' requested here _VSTD::generate_canonical<result_type, ^ ...

because the argument to the distribution sampler needs to satisfy the UniformRandomBitGenerator requirements. The following eliminates the error:

struct gamma_engine { typedef uint64_t result_type; static constexpr uint64_t min() { return 0; } static constexpr uint64_t max() { return std::numeric_limits<uint64_t>::max(); } uint64_t operator()() { return crypto::rand<uint64_t>(); } } engine;

stoffu · 2018-07-02T06:54:51Z

src/wallet/wallet2.cpp

+          if (amount == 0 && has_rct_distribution)
+          {
+            // gamma distribution
+            do i = pick_gamma(); while (i >= num_outs);


This means the pre/post-fork segregation is deactivated for this scheme. Is this intentional?

It was not, it was coded before that change.

stoffu · 2018-07-03T03:36:15Z

src/wallet/wallet2.cpp

+    struct gamma_engine
+    {
+      uint64_t min() const { return 0; }
+      uint64_t max() const { return std::numeric_limits<uint64_t>::max(); }


min and max need to be static according to the specification. OSX builds are failing: https://build.getmonero.org/builders/monero-static-osx-10.13/builds/762/steps/compile/logs/stdio

constexpr seems also necessary, because I had to add it to make it compile for OSX.

stoffu · 2018-07-03T04:05:59Z

src/wallet/wallet2.cpp

+          if (amount == 0 && has_rct_distribution)
+          {
+            // gamma distribution
+            if (num_found -1 < recent_outputs_count + pre_fork_outputs_count)


In this case recent_outputs_count is known to be 0, right? Why bother adding?

Because it matches the ordering so it's a lot clearer.

moneromooo-monero · 2018-07-03T17:02:17Z

Does it buld for you without typedef uint64_t result_type; ? Adding this here makes CLANG error out with "error: unused typedef 'result_type' [-Werror,-Wunused-local-typedef]". Alternatively, I could replace the uint64_t in the return types with result_type though, if that also works for you.

Edit: I've just pushed a version with that second alternative.

as per "An Empirical Analysis of Linkability in the Monero Blockchain", by Miller et al.

stoffu · 2018-07-03T22:43:50Z

Not adding result_type does work, yes.

34d4b79 wallet2: use a gamma distribution to pick fake outs (moneromooo-monero)

vtnerd reviewed Apr 3, 2018

View reviewed changes

moneromooo-monero force-pushed the gamma branch from 15301d8 to 6dbbbe7 Compare June 27, 2018 10:34

naughtyfox reviewed Jun 29, 2018

View reviewed changes

stoffu reviewed Jul 2, 2018

View reviewed changes

moneromooo-monero force-pushed the gamma branch 2 times, most recently from b722e20 to 24b2313 Compare July 2, 2018 15:23

stoffu reviewed Jul 3, 2018

View reviewed changes

moneromooo-monero force-pushed the gamma branch from 24b2313 to 1664fee Compare July 3, 2018 16:49

dternyak mentioned this pull request Jul 3, 2018

Use gamma distribution to pick fake outputs MyCryptoHQ/xmr-core#6

Open

1 task

moneromooo-monero force-pushed the gamma branch from 1664fee to 175fca7 Compare July 3, 2018 16:57

wallet2: use a gamma distribution to pick fake outs

34d4b79

as per "An Empirical Analysis of Linkability in the Monero Blockchain", by Miller et al.

moneromooo-monero force-pushed the gamma branch from 175fca7 to 34d4b79 Compare July 3, 2018 17:11

stoffu approved these changes Jul 3, 2018

View reviewed changes

luigi1111 merged commit 34d4b79 into monero-project:master Aug 15, 2018

luigi1111 added a commit that referenced this pull request Aug 15, 2018

Merge pull request #3528

4cde4cb

34d4b79 wallet2: use a gamma distribution to pick fake outs (moneromooo-monero)

aeon1234 mentioned this pull request Aug 27, 2018

[suggestion/request] automatic churn #4305

Open

This was referenced Feb 19, 2019

Upstream merge 4 aeonix/aeon#99

Closed

Upstream merge 4 [fixed] aeonix/aeon#100

Closed

stoffu mentioned this pull request Mar 12, 2019

Upstream merge 4 aeonix/aeon#106

Merged

jeffro256 mentioned this pull request May 23, 2023

[Vulnerability Disclosure] Post-Mortem of 10-Block-Old Decoy Selection Bug 2023 #8872

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet2: use a gamma distribution to pick fake outs #3528

wallet2: use a gamma distribution to pick fake outs #3528

moneromooo-monero commented Mar 30, 2018

vtnerd left a comment •

edited

Loading

vtnerd Apr 3, 2018

vtnerd Apr 3, 2018

moneromooo-monero Apr 4, 2018

stoffu Jul 2, 2018

vtnerd Apr 3, 2018

moneromooo-monero Apr 4, 2018

moneromooo-monero commented Apr 4, 2018

naughtyfox Jun 29, 2018

moneromooo-monero Jun 29, 2018

stoffu Jul 2, 2018

stoffu Jul 2, 2018

moneromooo-monero Jul 2, 2018

stoffu Jul 3, 2018 •

edited

Loading

stoffu Jul 3, 2018

moneromooo-monero Jul 3, 2018

stoffu Jul 3, 2018

moneromooo-monero commented Jul 3, 2018 •

edited

Loading

stoffu commented Jul 3, 2018

wallet2: use a gamma distribution to pick fake outs #3528

wallet2: use a gamma distribution to pick fake outs #3528

Conversation

moneromooo-monero commented Mar 30, 2018

vtnerd left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moneromooo-monero commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoffu Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moneromooo-monero commented Jul 3, 2018 • edited Loading

stoffu commented Jul 3, 2018

vtnerd left a comment •

edited

Loading

stoffu Jul 3, 2018 •

edited

Loading

moneromooo-monero commented Jul 3, 2018 •

edited

Loading